gh-145176 Move Emscripten files into Platforms/emscripten#145806
gh-145176 Move Emscripten files into Platforms/emscripten#145806hoodmane wants to merge 4 commits intopython:mainfrom
Conversation
hoodmane
requested review from
AA-Turner,
brettcannon,
emmatyping,
erlend-aasland and
freakboy3742
as code owners
|
!buildbot Emscripten |
|
🤖 New build scheduled with the buildbot fleet by @hoodmane for commit e8221bd 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F145806%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
!buildbot Emscripten |
|
🤖 New build scheduled with the buildbot fleet by @hoodmane for commit 6257cd3 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F145806%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
The bones of this all look straightforward; I've flagged a couple of things inline, including the directory simplification that I raised during the last PR.
There's also evidently still an issue with testing - I'm guessing it's the path depth in playwright.config.ts. That will also need to be updated for the path simplification - and if there's a way to make it work with a custom cross-build folder, that would be even better.
| "host_build_dir": host_triple_dir / "build", | ||
| "host_dir": host_triple_dir / "build" / "python", |
There was a problem hiding this comment.
This was the suggestion I made during the final reviews of the last PR:
| "host_build_dir": host_triple_dir / "build", | |
| "host_dir": host_triple_dir / "build" / "python", | |
| "host_build_dir": host_triple_dir | |
| "host_dir": host_triple_dir / "python", |
There was a problem hiding this comment.
I think this breaks the build bot? So the idea is:
- Merge this as is
- Add run subcommand
- Change build bot to use new Platforms/emscripten path and the run subcommand
Then we could make this change although there may be other places where the build bot depends on the current build path.
There was a problem hiding this comment.
Yes, it would break the buildbot; I hadn't considered that if we make this change, then make the buildbot use the new entry script, the buildbot sensitive to the build path goes away.
So yes - your landing plan makes sense to me.
| if __name__ == "__main__": | ||
| main() | ||
| import pathlib | ||
| import runpy |
There was a problem hiding this comment.
We should probably include a warning mirroring the WASI one:
| import runpy | |
| import runpy | |
| print( | |
| "⚠️ WARNING: This script is deprecated and slated for removal in Python 3.20; " | |
| "execute the `Platforms/emscripten/` directory instead (i.e. `python Platforms/emscripten`)\n", | |
| file=sys.stderr, | |
| ) |
There was a problem hiding this comment.
Do we need to keep it around that long?
There was a problem hiding this comment.
The date is the same one in the WASI folder - I don't know what drove @brettcannon's decision in that. I agree it seems super conservative.
There was a problem hiding this comment.
I guess it doesn't hurt, though I think moving the path where we build from cross-build/emscripten/build/python is going to break anyone who would be broken by removing this script so we should decide what our backwards compatibility aims are and be consistent.
There was a problem hiding this comment.
I was thinking we could delete it as soon as we update the build bot personally.
There was a problem hiding this comment.
I've been super conservative since the code is just sitting there and not in anyone's way. Plus there's a bit of simplicity with the buildbots when supporting older versions prior to the fix.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
4 participants
To see the diff in
__main__.pylook at just the first commit.